-
Notifications
You must be signed in to change notification settings - Fork 11
PG-1813 Make WAL keys TLI aware #509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (82.14%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## TDE_REL_17_STABLE #509 +/- ##
=====================================================
+ Coverage 82.00% 82.14% +0.13%
=====================================================
Files 24 25 +1
Lines 3162 3186 +24
Branches 514 518 +4
=====================================================
+ Hits 2593 2617 +24
Misses 460 460
Partials 109 109
🚀 New features to boost your workflow:
|
contrib/pg_tde/t/wal_key_tli.pl
Outdated
|
||
run_test('remote'); | ||
|
||
done_testing(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline at end of file.
@@ -166,7 +185,8 @@ TDEXLogShmemInit(void) | |||
|
|||
typedef struct EncryptionStateData | |||
{ | |||
XLogRecPtr enc_key_lsn; /* to sync with reader */ | |||
XLogRecPtr enc_key_tli; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not new, but this double EncryptionStateData seems strage to me - postgres doesn't have support for the atomic types in the frontend, or why do we have to duplicate them?
Also, now we also have a sizedifference, tli is 32 bit in the backend code, and 64 in the frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Yes, frontend doesn't support atomics:
...
#define ATOMICS_H
#ifdef FRONTEND
#error "atomics.h may not be included from frontend code"
#endif
...
- My bad, it must be a
TimeLineID
type. Fixed.
Before this commit, WAL keys didn't mind TLI at all. But after pg_rewind, for example, pg_wal/ may contain segments from two timelines. And the wal reader choosing the key may pick the wrong one because LSNs of different TLIs may overlap. There was also another bug: There is a key with the start LSN 0/30000 in TLI 1. And after the start in TLI 2, the wal writer creates a new key with the SN 0/30000, but in TLI 2. But the reader wouldn't fetch the latest key because w/o TLI, these are the same. This commit adds TLI to the Internal keys and makes use of it along with LSN for key compares.
Before this commit, WAL keys didn't mind TLI at all. But after pg_rewind, for example, pg_wal/ may contain segments from two timelines. And the wal reader choosing the key may pick the wrong one because LSNs of different TLIs may overlap. There was also another bug: There is a key with the start LSN 0/30000 in TLI 1. And after the start in TLI 2, the wal writer creates a new key with the SN 0/30000, but in TLI 2. But the reader wouldn't fetch the latest key because w/o TLI, these are the same.
This PR adds TLI to the Internal keys and makes use of it along with LSN for key compares.
It replaces #491. The code is identical + addressed the comment. It was just easier to create a new commit than bother with the rebase of latest changes.
Fixes: https://perconadev.atlassian.net/browse/PG-1813